backend/tests/services/readers/test_kb_permission_resolver.py (1)

268-275: Dead assignment: return_value is immediately overridden by side_effect.

Line 268 sets first.return_value = mock_kb, but line 270 then sets first.side_effect = [mock_kb, None]. Since side_effect takes precedence over return_value, line 268 has no effect and can be misleading to readers. Consider removing it.

♻️ Suggested fix

-        # Simulate KB query returning mock_kb
-        mock_query.return_value.filter.return_value.first.return_value = mock_kb
-        # Simulate no explicit permission
-        mock_query.return_value.filter.return_value.first.side_effect = [
+        # KB lookup returns mock_kb, subsequent explicit-permission lookup returns None
+        mock_query.return_value.filter.return_value.first.side_effect = [
             mock_kb,   # KB lookup
             None,      # no explicit ResourceMember
         ]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/tests/services/readers/test_kb_permission_resolver.py` around lines
268 - 275, Remove the redundant assignment to
mock_query.return_value.filter.return_value.first.return_value (the line that
sets it to mock_kb) because you immediately override behavior by setting
mock_query.return_value.filter.return_value.first.side_effect = [mock_kb, None];
keep only the side_effect assignment so first() returns mock_kb then None, and
update any test comments to reflect that ResourceMember lookup is simulated via
the side_effect; locate these in the tests in test_kb_permission_resolver.py
around the mock_query and mock_resolver setup.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/app/services/readers/kb_permissions.py (1)

134-148: Silent fallback when wrap() returns a falsy value.

If an extension's wrap(base) returns None (or any falsy value), the loader silently drops it and returns base without any log, making misconfigured extensions hard to diagnose. Either log a warning or use an explicit is None check.

♻️ Suggested tweak

-        ext = importlib.import_module(f"{settings.SERVICE_EXTENSION}.kb_permissions")
-        result = ext.wrap(base)
-        if result:
-            logger.info("KB permission resolver extension loaded")
-            return result
+        ext = importlib.import_module(f"{settings.SERVICE_EXTENSION}.kb_permissions")
+        result = ext.wrap(base)
+        if result is None:
+            logger.warning(
+                "kb_permissions extension wrap() returned None; using default resolver"
+            )
+        else:
+            logger.info("KB permission resolver extension loaded")
+            return result

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/readers/kb_permissions.py` around lines 134 - 148, The
loader currently treats any falsy return from ext.wrap(base) as a success and
silently falls back to base; update the logic in the kb_permissions
extension-loading block to detect falsy returns from ext.wrap(base) (preferably
checking "is None" if None is the intended sentinel) and log a warning
(including the extension name via settings.SERVICE_EXTENSION) when ext.wrap
returns None or another unexpected falsy value, before returning base; keep the
existing ImportError/Exception handlers and only consider the ext.wrap return
value for this additional diagnostic logging.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/app/services/readers/kb_permissions.py (1)

161-172: Lazy singleton initialization is not thread-safe.

_get() performs a check-then-set on self._instance without a lock. Under concurrent first access (e.g., two request threads hitting list_knowledge_bases and get_user_kb_permission simultaneously at startup), _create_resolver() can run more than once — which also re-imports the extension module and re-logs the "extension loaded" message. In practice importlib.import_module is cached so the extra cost is small, but it's worth a threading.Lock (double-checked locking) or eager initialization at import time to make the "called only once" guarantee that the tests assert actually hold in production.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/readers/kb_permissions.py` around lines 161 - 172, The
lazy singleton _LazyReader is not thread-safe: _get performs a check-then-set on
_instance without synchronization, so concurrent first accesses can call
_create_resolver() multiple times; fix by adding a threading.Lock as a
class/member (e.g., _init_lock) and implement double-checked locking inside _get
(check _instance, acquire lock, check again, call _create_resolver and assign to
_instance) so only one resolver is created, or alternatively perform eager
initialization by calling _create_resolver() at module import and initializing
_instance once; ensure references in __getattr__ still call _get() and preserve
existing symbols (_LazyReader, _get, _instance, _create_resolver, __getattr__).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/app/services/readers/kb_permissions.py (1)

135-154: Consider narrowing the fallback error log to include exc_info.

except Exception as e correctly catches the miscellaneous failure modes (missing wrap, wrap raising, etc.), but the warning at line 152 only serializes str(e), which often loses the traceback for non-ImportError failures originating inside the extension. For an extension-loading seam that is otherwise silent, passing exc_info=True on that branch makes diagnosing misbehaving extensions considerably easier without changing behavior.

🛠️ Suggested tweak

-    except Exception as e:
-        logger.warning(f"Failed to load kb_permissions extension: {e}")
+    except Exception as e:  # noqa: BLE001 - extension isolation boundary
+        logger.warning(
+            "Failed to load kb_permissions extension: %s", e, exc_info=True
+        )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/readers/kb_permissions.py` around lines 135 - 154, The
catch-all exception handler that logs "Failed to load kb_permissions extension:
{e}" should include the full traceback for easier debugging; in the except
Exception block that handles failures from importing or calling
settings.SERVICE_EXTENSION.kb_permissions.wrap, change the logger.warning call
that currently serializes only str(e) to pass exc_info=True (keeping the same
message) so the logger records the exception stack trace when wrap() or module
import fails.

backend/app/services/knowledge/knowledge_service.py (1)

430-437: Silence Ruff BLE001 with an explanatory noqa.

The broad except Exception here is intentional — the whole point is to prevent a faulty extension from taking down the core listing path — but Ruff will keep flagging it (BLE001). Consider adding a # noqa: BLE001 with a short rationale so the intent is explicit and the lint stays clean.

✏️ Suggested diff

             try:
                 ext_kb_ids = kb_permission_resolver.get_accessible_kb_ids(db, user_id)
-            except Exception as e:
+            except Exception as e:  # noqa: BLE001 - never let extension break core listing
                 logger.warning(
                     f"kb_permissions extension get_accessible_kb_ids failed: {e}; "
                     "falling back to empty list"
                 )
                 ext_kb_ids = []

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/knowledge/knowledge_service.py` around lines 430 - 437,
Add a targeted `# noqa: BLE001` to the broad except block that wraps the call to
kb_permission_resolver.get_accessible_kb_ids so Ruff stops flagging the
intentional catch; update the except line in the try/except that assigns
ext_kb_ids (the block that logs via logger.warning and falls back to ext_kb_ids
= []) to include `# noqa: BLE001` and a short inline rationale (e.g.,
"intentional broad except to isolate faulty extension"), keeping the rest of the
handler (logger.warning and fallback to empty list) unchanged.